-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add command to change OpenJDK versions from 11 to 8 #42
Add command to change OpenJDK versions from 11 to 8 #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I would probably make the comments in a separate commit to keep the PR cleaner
- That command is not available on newer versions of java (e.g the default 11 - gonna need to find something different. Additionally, you may need to install the actual version of Java before you can set it. In terms of actually checking the version, maybe some logic to stop the command from executing if the version matches what they're trying to change it to would be nice
- I'm not sure about the JAVA_HOME significance, but CircleCI's environment can be different than most; the PATH could be different
- I would also put a test into the actual configuration to see if this works
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Added check to ensure that the requested java version is not the currently set version, as well as added code to set the JAVA_HOME and PATH variables following the installation of a new java version |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Here is a link to a green build of the current PR: The current PR has coverage for switch to OpenJDK versions 8 and 11, as well as for installing other versions of OpenJDK such as version 13 |
i think it looks fine, but i would want to get a second review as well |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good PR. Needs a few changes but otherwise looks good.
I originally envisioned this feature as a bash script in the image itself as opposed to a command in the orb. This method works too. The added advantage as well is now this command will show up on the orb docs page making it more discoverable. Awesome.
I also LOVE how a test for this new feature was included in this PR.
.circleci/test-deploy.yml
Outdated
Version to use in the change-java-version job | ||
executor: | ||
name: android/android-docker | ||
tag: "2021.10.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android VM images are released every 1-2 months. We probably want to use a bit of a newer image here.
.circleci/test-deploy.yml
Outdated
if [ "$JAVA_VER" -ne <<parameters.java-version>> ] || [ "$JAVAC_VER" -ne <<parameters.java-version>> ]; then | ||
exit 1 | ||
else | ||
exit 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 41 and 42 are unnecessary here. Since this shell ends right when the "command" key ends, it will automatically return an exit code of 0 anyway.
src/commands/change-java-version.yml
Outdated
exit 1 | ||
else | ||
exit 0 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note as above.
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
name: "Test OpenJDK version change" | ||
executor: | ||
name: android/android-docker | ||
tag: "2021.10.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would still be nice to have updated but I won't hold back this PR for it.
86f3c38
86c23db
to
86f3c38
Compare
Your development orb has been published. It will expire in 30 days. |
Your development orb has been published. It will expire in 30 days. |
737ab19
to
121f665
Compare
Your development orb has been published. It will expire in 30 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Added a new job to allow switch OpenJDK versions per this issue
Description
Wrote a command with 3 steps, the first to check the current OpenJDK version, the second changes the default OpenJDK version of the current image to OpenJDK v8, and the third step confirms the change by running java -version once more
Also, added a parameter that will allow you to change OpenJDK versions, default: '1.8.0'. To check available OpenJDK versions, run
update-java-alternatives --list